Remove requirement that forces symmetric and transitive PartialEq impls to exist #81198

dtolnay · 2021-01-19T22:36:14Z

Counterexample of symmetry:

If you have an impl like:

impl<T> PartialEq<T> for Ident
where
    T: ?Sized + AsRef<str>

then Rust will not even allow the symmetric impl to exist.

error[E0210]: type parameter `T` must be covered by another type when it appears before the first local type (`Ident`)
 --> src/main.rs:9:6
  |
9 | impl<T> PartialEq<Ident> for T where T: ?Sized + AsRef<str> {
  |      ^ type parameter `T` must be covered by another type when it appears before the first local type (`Ident`)
  |
  = note: implementing a foreign trait is only possible if at least one of the types for which it is implemented is local, and no uncovered type parameters appear before that first local type
  = note: in this case, 'before' refers to the following order: `impl<..> ForeignTrait<T1, ..., Tn> for T0`, where `T0` is the first and `Tn` is the last

Counterexample of transitivity:

Consider these two existing impls from regex and clap:

// regex

/// An inline representation of `Option<char>`.
pub struct Char(u32);

impl PartialEq<char> for Char {
    fn eq(&self, other: &char) -> bool {
        self.0 == *other as u32
    }
}

// clap

pub(crate) enum KeyType {
    Short(char),
    Long(OsString),
    Position(u64),
}

impl PartialEq<char> for KeyType {
    fn eq(&self, rhs: &char) -> bool {
        match self {
            KeyType::Short(c) => c == rhs,
            _ => false,
        }
    }
}

It's nice to be able to add PartialEq<proc_macro::Punct> for char in libproc_macro (#80595), but it makes no sense to force an impl PartialEq<Punct> for Char and impl PartialEq<Punct> for KeyType in regex and clap in code that otherwise has nothing to do with proc macros.

@rust-lang/libs

…ls to exist

rust-highfive · 2021-01-19T22:36:19Z

r? @sfackler

(rust-highfive has picked a reviewer for you, use r? to override)

dtolnay · 2021-01-19T22:36:39Z

@rfcbot fcp merge

rfcbot · 2021-01-19T22:36:40Z

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

pthariensflame · 2021-01-20T00:14:50Z

I think we should at least have something that says symmetry and transitivity are encouraged if they can be done without undue difficulty.

sfackler · 2021-01-20T20:09:53Z

Agreed that it would be good to recommend the symmetric impl when possible.

rfcbot · 2021-01-20T20:14:07Z

🔔 This is now entering its final comment period, as per the review above. 🔔

pthariensflame · 2021-01-21T00:06:14Z

@sfackler Can you register a Concern, then? (Can I?)

KodrAus · 2021-01-21T00:29:56Z

This seems reasonable to me. I could imagine we could find ways to help users around symmetry and transitivity issues through error messages or explicitly codified symmetric or transitive functions that polyfill missing impls.

error[E0369]: binary operation `==` cannot be applied to type `B`
  --> src/main.rs:70:15
   |
70 |     let _ = b == a;
   |             - ^^ - A
   |             |
   |             B
   |
   = note: an implementation of `std::cmp::PartialEq` is available for `A`, try `let _ = a == b;`

and for impl PartialEq bounds in functions:

error[E0277]: can't compare `B` with `A`
  --> src/main.rs:71:33
   |
17 | fn needs_partial_eq(a: A, lhs: impl PartialEq<A>) -> bool {
   |                                     ------------ required by this bound in `needs_partial_eq`
...
71 |     let _ = needs_partial_eq(A, B);
   |                                 ^ no implementation for `B == A`
   |
   = help: the trait `PartialEq<A>` is not implemented for `B`, but `PartialEq<B>` is implemented for `A`
   = help: try `let _ = needs_partial_eq(A, std::cmp::symmetric(B));`

Playground

dtolnay · 2021-01-21T01:39:30Z

I think we should at least have something that says symmetry and transitivity are encouraged if they can be done without undue difficulty.

I don't necessarily agree with this. Skimming the clap PartialEq code that's mentioned in the transitivity counterexample above (https://github.com/clap-rs/clap/blob/a0269a41d4abaf4b0a9ec4f9a059fe62ea0ba3a7/src/mkeymap.rs#L34-L68), it's hard to say it would be "unduly difficult" to write the full set of reverse impls, but at the same time I really see no benefit of clap doing that. I'd rather they just write whatever impls are needed, and not write whatever impls are not needed.

If your recommendation was intended to apply to public APIs only, that seems more suited to API design guidelines than any official requirement of the trait.

KodrAus · 2021-01-22T00:14:12Z

On the topic of public APIs specifically I opened up a discussion in the API Guidelines for it: rust-lang/api-guidelines#235

dylni · 2021-01-23T19:50:36Z

Aside from when it's not possible, is there ever a reason to not make an impl symmetric? If they're not, comparisons need to have a specific order, which should only be a style decision. This can make assert_eq inconsistent for tests.

That's why more PartialEq impls were added later to compare Vec and slice: #71660

dtolnay · 2021-01-23T19:54:56Z

Aside from when it's not possible, is there ever a reason to not make an impl symmetric?

Yes, there is. If I'm only going to perform the comparisons in one direction in my codebase, I'm not going to write 2 impls, one of which is dead code. I pointed this out in #81198 (comment).

dylni · 2021-01-23T20:03:27Z

Sorry for not reading the comments well enough. I was looking closer through #80595.

If I'm only going to perform the comparisons in one direction in my codebase, I'm not going to write 2 impls, one of which is dead code.

I can understand this, even though I don't think I would do it myself. Requiring both to exist does seem unnecessary, but shouldn't a recommendation stay on this documentation page? If it's in the API guidelines, it might not be seen by those that want to learn more about the trait.

varkor · 2021-01-23T20:42:18Z

Perhaps it would be worth being able to derive the symmetry after having implemented one direction. I agree that it's better to have both if possible (and the inconsistency in the past has caused problems with generalising some of the existing library methods).

rfcbot · 2021-01-30T20:16:25Z

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

m-ou-se · 2021-01-30T21:12:50Z

@bors r+ rollup

bors · 2021-01-30T21:12:51Z

📌 Commit 8758083 has been approved by m-ou-se

…as-schievink Rollup of 18 pull requests Successful merges: - rust-lang#78044 (Implement io::Seek for io::Empty) - rust-lang#79285 (Stabilize Arc::{increment,decrement}_strong_count) - rust-lang#80053 (stabilise `cargo test -- --include-ignored`) - rust-lang#80279 (Implement missing `AsMut<str>` for `str`) - rust-lang#80470 (Stabilize by-value `[T; N]` iterator `core::array::IntoIter`) - rust-lang#80945 (Add Box::downcast() for dyn Any + Send + Sync) - rust-lang#81048 (Stabilize `core::slice::fill_with`) - rust-lang#81198 (Remove requirement that forces symmetric and transitive PartialEq impls to exist) - rust-lang#81422 (Account for existing `_` field pattern when suggesting `..`) - rust-lang#81472 (Clone entire `TokenCursor` when collecting tokens) - rust-lang#81484 (Optimize decimal formatting of 128-bit integers) - rust-lang#81491 (Balance sidebar `Deref` cycle check with main content) - rust-lang#81509 (Add a regression test for ICE of bad_placeholder_type) - rust-lang#81547 (Edit rustc_typeck top-level docs) - rust-lang#81550 (Replace predecessor with range in collections documentation) - rust-lang#81558 (Fix ascii art text wrapping in mobile) - rust-lang#81562 (Clarify that InPlaceIterable guarantees extend to all advancing iterator methods.) - rust-lang#81563 (Improve docblock readability on small screen) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

pickfire · 2021-02-04T01:35:17Z

library/core/src/cmp.rs

+/// - **Symmetric**: if `A: PartialEq<B>` and `B: PartialEq<A>`, then **`a == b`
+///   implies `b == a`**; and
+///
+/// - **Transitive**: if `A: PartialEq<B>` and `B: PartialEq<C>` and `A:


If this part is updated for styling then Eq part should also be updated. Let me send a pull request.

Remove requirement that forces symmetric and transitive PartialEq imp…

8758083

…ls to exist

dtolnay added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jan 19, 2021

rust-highfive assigned sfackler Jan 19, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 19, 2021

rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jan 19, 2021

dtolnay mentioned this pull request Jan 19, 2021

impl PartialEq<Punct> for char; symmetry for #78636 #80595

Merged

This comment has been minimized.

Sign in to view

rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Jan 20, 2021

rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jan 20, 2021

RalfJung mentioned this pull request Jan 22, 2021

Document "latent requirements" of PartialOrd #50230

Closed

rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jan 30, 2021

rfcbot added the to-announce Announce this issue on triage meeting label Jan 30, 2021

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 30, 2021

jonas-schievink mentioned this pull request Jan 31, 2021

Rollup of 18 pull requests #81578

Merged

bors merged commit 13b3294 into rust-lang:master Jan 31, 2021

rustbot added this to the 1.51.0 milestone Jan 31, 2021

pickfire reviewed Feb 4, 2021

View reviewed changes

dtolnay deleted the partialeq branch February 4, 2021 04:31

petrochenkov mentioned this pull request Feb 7, 2021

Fix derived PartialOrd operators #81384

Merged

apiraino removed the to-announce Announce this issue on triage meeting label Feb 11, 2021

sfackler mentioned this pull request Jul 11, 2021

Std implementations of PartialOrd are violating the conditions regarding transitivity and symmetry. And the transitivity requirements are i̶m̶p̶o̶s̶s̶i̶b̶l̶e̶ hard to ensure in a multi-crate ecosystem anyways. #87067

Closed

JanBeh mentioned this pull request Oct 22, 2022

docs: Correct terminology in std::cmp #103046

Merged

This was referenced Nov 20, 2023

PartialOrd: transitivity and duality are required only if the corresponding impls exist #118108

Closed

PartialEq, PartialOrd: update and synchronize handling of transitive chains #115386

Merged

Mark-Simulacrum mentioned this pull request Jan 14, 2024

Enforce informal properties of traits such as PartialEq #46946

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove requirement that forces symmetric and transitive PartialEq impls to exist #81198

Remove requirement that forces symmetric and transitive PartialEq impls to exist #81198

dtolnay commented Jan 19, 2021

rust-highfive commented Jan 19, 2021

dtolnay commented Jan 19, 2021

rfcbot commented Jan 19, 2021 •

edited by KodrAus

Loading

pthariensflame commented Jan 20, 2021

This comment has been minimized.

sfackler commented Jan 20, 2021

rfcbot commented Jan 20, 2021

pthariensflame commented Jan 21, 2021

KodrAus commented Jan 21, 2021 •

edited

Loading

dtolnay commented Jan 21, 2021

KodrAus commented Jan 22, 2021

dylni commented Jan 23, 2021 •

edited

Loading

dtolnay commented Jan 23, 2021

dylni commented Jan 23, 2021

varkor commented Jan 23, 2021

rfcbot commented Jan 30, 2021

m-ou-se commented Jan 30, 2021

bors commented Jan 30, 2021

pickfire Feb 4, 2021

Remove requirement that forces symmetric and transitive PartialEq impls to exist #81198

Remove requirement that forces symmetric and transitive PartialEq impls to exist #81198

Conversation

dtolnay commented Jan 19, 2021

Counterexample of symmetry:

Counterexample of transitivity:

rust-highfive commented Jan 19, 2021

dtolnay commented Jan 19, 2021

rfcbot commented Jan 19, 2021 • edited by KodrAus Loading

pthariensflame commented Jan 20, 2021

This comment has been minimized.

sfackler commented Jan 20, 2021

rfcbot commented Jan 20, 2021

pthariensflame commented Jan 21, 2021

KodrAus commented Jan 21, 2021 • edited Loading

dtolnay commented Jan 21, 2021

KodrAus commented Jan 22, 2021

dylni commented Jan 23, 2021 • edited Loading

dtolnay commented Jan 23, 2021

dylni commented Jan 23, 2021

varkor commented Jan 23, 2021

rfcbot commented Jan 30, 2021

m-ou-se commented Jan 30, 2021

bors commented Jan 30, 2021

pickfire Feb 4, 2021

Choose a reason for hiding this comment

rfcbot commented Jan 19, 2021 •

edited by KodrAus

Loading

KodrAus commented Jan 21, 2021 •

edited

Loading

dylni commented Jan 23, 2021 •

edited

Loading